add resource permission checks #1711

petrkalos · 2024-11-21T13:59:26Z

Feature or Bugfix

Feature

Detail

introducing a test that is going through all the nested SQL queries and assert that ResourcePolicyService.check_user_resource_permission have been called with the expected permission name OR explicitly ignore the test.
- New subqueries will be tested automatically and fail if the expected permission is missing
- Removed queries will make the test suite fail to avoid keeping stale permissions
UI: Make handling responses (i.e ListDatasets, GetDataset) more tolerant to missing information (i.e missing Stack) by doing conditional rendering.
Example usecase: A dataset is being shared by a user but only owners have permissions to see stack and environment info.
Override config.json and enable all modules when running the tests. As a result checkov now synths the pipeline module that throws some errors (added in the baseline). @noah-paige
Make TestClient more tolerant to GQLErrors previously it would always throw if errors, now it will throw if there are only erros (and no data) allowing for partial information to be returned to the caller

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

Does this PR introduce or modify any input fields or queries - this includes
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
- Is the input sanitized?
- What precautions are you taking before deserializing the data you consume?
- Is injection prevented by parametrizing queries?
- Have you ensured no eval or similar functions are used?
Does this PR introduce any functionality or component that requires authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
- Are you logging failed auth attempts?
Are you using or adding any cryptographic features?
- Do you use a standard proven implementations?
- Are the used keys controlled by the customer? Where are they stored?
Are you introducing any new policies/roles/users?
- Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

tests_new/integration_tests/client.py

tests/test_tenant_unauthorized.py

frontend/src/modules/Folders/views/FolderView.js

backend/dataall/modules/s3_datasets/api/storage_location/resolvers.py

backend/dataall/modules/s3_datasets/services/dataset_service.py

backend/dataall/core/organizations/services/organization_service.py

backend/dataall/modules/datapipelines/services/datapipelines_service.py

tests/test_tenant_unauthorized.py

tests/test_permissions.py

noah-paige

Mostly minor comments on updates to ignore cases

Couple of high level Qs:

(1) Is there any way to assess that the FE client properly handles all partial data returns appropriately?
(2 - Not necessarily in this PR but …) should we change everywhere on our FE client side to check if there is no data rather than if no errors (i.e. if (!response.errors)) ++ dispatch all error if exist rather than just the first (i.e. dispatch({ type: SET_ERROR, error: response.errors[0].message });)

backend/dataall/core/vpc/services/vpc_service.py

tests/test_permissions.py

petrkalos · 2024-12-05T10:18:39Z

Mostly minor comments on updates to ignore cases

Couple of high level Qs:

(1) Is there any way to assess that the FE client properly handles all partial data returns appropriately?

(2 - Not necessarily in this PR but …) should we change everywhere on our FE client side to check if there is no data rather than if no errors (i.e. if (!response.errors)) ++ dispatch all error if exist rather than just the first (i.e. dispatch({ type: SET_ERROR, error: response.errors[0].message });)

@noah-paige

I don't think so unless we visit all pages with all combinations of permissions. I think with the changes in this PR the impact is limited and easy to test manually. In Consistent get_<DATA_ASSET> permissions - Dashboards #1729 and in Consistent get_<DATA_ASSET> permissions - S3_Datasets #1727 therear a lot of those changes and @dlpzx is doing a manual validation. I think the risk isn't huge if something doesn't render properly it's easy to track and fix
I agree we should change this

backend/dataall/core/vpc/services/vpc_service.py

tests/test_permissions.py

noah-paige

One final comment on get_environment_networks changes

dlpzx · 2024-12-05T17:05:14Z

tests/permissions.py

+        resource_perm=GET_DATASET_TABLE, tenant_ignore=IgnoreReason.NOTREQUIRED
+    ),
+    field_id('DatasetTable', 'columns'): TestData(
+        resource_ignore=IgnoreReason.CUSTOM, tenant_ignore=IgnoreReason.USERLIMITED


CUSTOM as I understand it is that permissions are checked, but they are checked through their own mechanisms. Are there any checks happening in the columns resolver?

Yes I changed this after @noah-paige suggestion because it is checking the Confidentiality Classification...

@staticmethod def paginate_active_columns_for_table(uri: str, filter=None): context = get_context() with context.db_engine.scoped_session() as session: table: DatasetTable = DatasetTableRepository.get_dataset_table_by_uri(session, uri) dataset = DatasetRepository.get_dataset_by_uri(session, table.datasetUri) if ( ConfidentialityClassification.get_confidentiality_level(dataset.confidentiality) != ConfidentialityClassification.Unclassified.value ) and (dataset.SamlAdminGroupName not in context.groups and dataset.stewards not in context.groups): raise exceptions.UnauthorizedOperation( action='LIST_DATASET_TABLE_COLUMNS', message='User is not authorized to view Columns for Confidential datasets', ) return DatasetColumnRepository.paginate_active_columns_for_table(session, uri, filter)

tests/permissions.py

dlpzx

Left some nit changes but overall I think it is good to go. I was not strict with Redshift or Metadata Forms

@noah-paige

Feature * introducing a test that is going through all the nested SQL queries and assert that `ResourcePolicyService.check_user_resource_permission` have been called with the expected permission name OR explicitly ignore the test. * New subqueries will be tested automatically and fail if the expected permission is missing * Removed queries will make the test suite fail to avoid keeping stale permissions * UI: Make handling responses (i.e ListDatasets, GetDataset) more tolerant to missing information (i.e missing Stack) by doing conditional rendering. Example usecase: A dataset is being shared by a user but only owners have permissions to see stack and environment info. * Override config.json and enable all modules when running the tests. As a result checkov now synths the pipeline module that throws some errors (added in the baseline). @noah-paige * Make TestClient more tolerant to GQLErrors previously it would always throw if errors, now it will throw if there are only erros (and no data) allowing for partial information to be returned to the caller Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

petrkalos force-pushed the feature/nested_resolver_tests branch 3 times, most recently from 891b681 to 02a7fa9 Compare November 28, 2024 15:00

petrkalos marked this pull request as ready for review November 29, 2024 13:18

petrkalos requested review from noah-paige and dlpzx November 29, 2024 13:19